Custom flag for validator log table #34
Conversation
📝 WalkthroughWalkthroughAdds validator configuration CRUD, DB model and migrations, test support for validator configs, and a flag to include successful validator logs in persisted guardrails logs; plus related refactors, import path adjustments, and test/CI env tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as Guardrails API
participant ValidatorRouter as ValidatorConfigs Router
participant CRUD as ValidatorConfig CRUD
participant DB as Database
participant LogStore as ValidatorLog persistence
Client->>API: POST /guardrails/validators/configs (create)
API->>ValidatorRouter: route handler
ValidatorRouter->>CRUD: create(payload)
CRUD->>DB: INSERT validator_config
DB-->>CRUD: OK (row)
CRUD-->>ValidatorRouter: flattened response
ValidatorRouter-->>API: 201 Created
API-->>Client: response
Client->>API: POST /guardrails/validate (run_guardrails, include_all_validator_logs?)
API->>API: _validate_with_guard(..., include_all_validator_logs)
API->>LogStore: add_validator_logs(iter_logs, include_all_validator_logs)
alt include_all_validator_logs == false
LogStore-->>DB: persist only failing validator logs
else
LogStore-->>DB: persist all validator logs (including PassResult)
end
LogStore-->>API: ack
API-->>Client: validation result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/tests/test_guardrails_api_integration.py (1)
1-196: 🛠️ Refactor suggestion | 🟠 MajorNo tests for the new
include_all_validator_logsfeature.The main feature of this PR — the
include_all_validator_logsflag — has no test coverage. Consider adding at least:
- A test with
include_all_validator_logs=Truethat verifies all validator logs (including passes) are persisted.- A test with the default (
False) that verifies only failure logs are persisted.This would confirm the filtering logic works end-to-end.
🤖 Fix all issues with AI agents
In `@backend/app/api/routes/guardrails.py`:
- Around line 180-181: Replace the explicit equality comparison to False in the
guardrails check with a boolean negation: in the if statement that currently
reads "if include_all_validator_logs == False and isinstance(result,
PassResult):", use "if not include_all_validator_logs and isinstance(result,
PassResult):" so the code uses idiomatic Python and avoids issues when
include_all_validator_logs is a truthy/falsy non-bool; update the condition
where include_all_validator_logs and PassResult are checked.
🧹 Nitpick comments (2)
backend/app/api/routes/guardrails.py (2)
24-28:include_all_validator_logsis a query parameter on a POST endpoint — consider moving it into the request body.Since
payloadis a Pydantic model parsed from the JSON body, FastAPI will treatinclude_all_validator_logsas a query parameter (e.g.,?include_all_validator_logs=true). Mixing body and query params on a POST can be surprising to API consumers. Consider adding this field toGuardrailRequestinstead for a cleaner contract.
163-163: Long signature — consider grouping parameters or wrapping across lines for readability.This is a minor readability nit. The function signature is quite long on a single line.
Suggested formatting
-def add_validator_logs(guard: Guard, request_log_id: UUID, validator_log_crud: ValidatorLogCrud, include_all_validator_logs: bool = False): +def add_validator_logs( + guard: Guard, + request_log_id: UUID, + validator_log_crud: ValidatorLogCrud, + include_all_validator_logs: bool = False, +):
rkritika1508
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@backend/app/api/routes/validator_configs.py`:
- Around line 23-28: The create_validator endpoint accepts organization_id and
project_id as unauthenticated query params—fix by enforcing tenant validation:
extract the authenticated tenant identifiers from the AuthDep (e.g., user or
token fields on the AuthDep dependency used by create_validator), compare them
to the incoming organization_id and project_id, and reject the request (raise an
appropriate HTTP 403/Forbidden) if they do not match; alternatively, remove
those query params and derive organization_id/project_id from the AuthDep
principal and pass only validated IDs into the CRUD layer so ValidatorCreate
handling and any calls that use session, create_validator, and the downstream
validator CRUD functions cannot operate across tenants.
In `@backend/app/schemas/validator_config.py`:
- Around line 24-30: ValidatorUpdate currently sets model_config =
ConfigDict(extra="forbid"), which blocks any arbitrary config keys in PATCH
payloads and prevents them reaching split_validator_payload/CRUD update; change
ValidatorUpdate to use model_config = ConfigDict(extra="allow") (matching
ValidatorCreate/ValidatorBase) so config fields in the incoming update are
accepted and can be merged by the existing split_validator_payload and the CRUD
update method.
- Around line 33-34: ValidatorResponse currently inherits only fields from
ValidatorBase and relies on extra="allow", so add explicit response fields to
the ValidatorResponse model to make them visible in OpenAPI: declare id: UUID4,
organization_id: UUID4, project_id: UUID4, created_at: datetime, and updated_at:
datetime (import UUID4 from pydantic and datetime from datetime) as attributes
on the ValidatorResponse class while keeping the existing inheritance from
ValidatorBase so the schema includes both base validator properties and these
persistent metadata fields.
In `@backend/app/tests/conftest.py`:
- Around line 13-17: When running tests the DB name can be empty and cause
destructive truncation of the wrong DB; add a validation in config.py (where
Settings or environment loading occurs) that checks if ENVIRONMENT == "testing"
and POSTGRES_DB is falsy/empty and, if so, raise a clear RuntimeError/ValueError
explaining that .env.test must be created and POSTGRES_DB must be set; also
ensure the error surfaces before create_engine/ test setup (reference
settings.SQLALCHEMY_DATABASE_URI and the POSTGRES_DB/ENVIRONMENT settings names)
and add a short comment/docs note telling devs to copy .env.test.example to
.env.test before running tests.
In `@backend/app/tests/test_validator_configs_integration.py`:
- Around line 60-82: The update_validator helper constructs its URL without the
trailing slash before DEFAULT_QUERY_PARAMS while get_validator and
delete_validator include it; update the URL construction in update_validator to
include the "/" between validator_id and DEFAULT_QUERY_PARAMS (same pattern as
get_validator/delete_validator) so the path becomes BASE_URL + validator_id +
"/" + DEFAULT_QUERY_PARAMS, ensuring consistent URL formatting across
create_validator/get_validator/update_validator/delete_validator and using the
same BASE_URL and DEFAULT_QUERY_PARAMS symbols.
In `@backend/app/utils.py`:
- Around line 26-28: The overlap check computing overlap = set(model_fields) &
set(config_fields) and raising ValueError is dead code because earlier routing
guarantees mutual exclusivity; remove the overlap calculation and the
conditional that raises the ValueError (or replace it with the intended
validation if you meant to check an external source), and ensure any intended
shadowing validation targets the correct input (e.g., compare
external_config_keys against model_fields instead of config_fields) so only
meaningful checks remain; search for model_fields, config_fields, and overlap to
locate and update the code.
🧹 Nitpick comments (12)
.env.test.example (1)
13-13: LGTM! Good improvements to the test database name.The change from hyphens to underscores and the addition of the
_testingsuffix are both good practices:
- Underscores avoid potential quoting issues with database identifiers
- The
_testingsuffix clearly distinguishes this from production databasesOptional nitpick: The static analysis tool suggests moving
POSTGRES_DBbeforePOSTGRES_SERVERfor alphabetical ordering. This is purely cosmetic and has no functional impact.📝 Optional reordering for consistency
# Postgres +POSTGRES_DB=kaapi_guardrails_testing +POSTGRES_PORT=5432 +POSTGRES_PASSWORD=postgres POSTGRES_SERVER=localhost -POSTGRES_DB=kaapi_guardrails_testing -POSTGRES_PORT=5432 POSTGRES_USER=postgres -POSTGRES_PASSWORD=postgresbackend/app/alembic/versions/002_added_validator_log.py (1)
8-8: Unused imports:SequenceandUnionare no longer referenced.Since the type annotations were removed from
down_revision,branch_labels, anddepends_on, these imports are dead code.Proposed fix
-from typing import Sequence, Unionbackend/app/api/routes/guardrails.py (2)
24-28: Consider whetherinclude_all_validator_logsshould be a query parameter on the POST endpoint.Currently this flag is a bare query parameter on the
POST /guardrails/endpoint. If this is intended as a per-request configuration, it might be cleaner to include it in theGuardrailRequestbody model instead, keeping all request-level config in one place.If it's meant as an operational/debug toggle, the query parameter approach is fine — just flagging for intentional design review.
13-13: Nit: extra whitespace in import.Line 13 has a double space before
RequestLogUpdate.Proposed fix
-from app.models.logging.request_log import RequestLogUpdate, RequestStatus +from app.models.logging.request_log import RequestLogUpdate, RequestStatusbackend/app/crud/validator_log.py (1)
1-1: Unused imports:UUIDanduuid4are not referenced in this file.The
createmethod receives a pre-constructedValidatorLog, so neitherUUIDnoruuid4is needed here.Proposed fix
-from uuid import UUID, uuid4backend/app/models/config/validator_config.py (1)
7-8: Redundant duplicate import ofField.
Fieldis imported twice — once asSQLField(Line 7) and once asField(Line 8). They are the same symbol. Consider using just one import with the alias where needed, or simply useFieldthroughout.♻️ Suggested cleanup
-from sqlmodel import Field as SQLField -from sqlmodel import SQLModel, Field +from sqlmodel import SQLModel, FieldThen on Line 50, replace
SQLField(withField(.backend/app/schemas/validator_config.py (1)
1-3: Unused imports.
datetime(Line 1) andUUID(Line 3) are imported but not used in the current schema definitions. They would become used ifValidatorResponseis updated per the suggestion above.backend/app/crud/validator_config.py (2)
59-63: Useis not Noneinstead of truthiness check for optional enum filters.
if stage:andif type:rely on truthiness. While enum members are truthy, usingis not Noneis more explicit and defensive against future enum changes or unexpected falsy values.♻️ Suggested fix
- if stage: + if stage is not None: query = query.where(ValidatorConfig.stage == stage) - if type: + if type is not None: query = query.where(ValidatorConfig.type == type)
105-107: Bareexcept Exceptionin update swallows context.The generic
except Exceptionblock on Lines 105–107 catches everything, rolls back, and re-raises. This is fine for ensuring rollback, but consider logging the exception before re-raising for observability, especially since this is a catch-all beyondIntegrityError.backend/app/api/routes/validator_configs.py (2)
19-31: POST endpoint should return HTTP 201 for resource creation.The create endpoint defaults to HTTP 200. Convention for resource creation is 201 Created.
♻️ Suggested fix
`@router.post`( "/", - response_model=APIResponse[ValidatorResponse] + response_model=APIResponse[ValidatorResponse], + status_code=201, )
81-94: DELETE returns 200 with a message body — consider 204 No Content.REST convention for a successful delete is typically HTTP 204 with no body. However, returning 200 with a message is also acceptable and may be preferred for consistency with the
APIResponsewrapper pattern used throughout.backend/app/tests/test_validator_configs.py (1)
79-90: CatchHTTPExceptioninstead of bareException.The CRUD layer raises
HTTPException(404, "Validator not found"). Catching genericExceptionweakens the test — it would pass even if the code raised a different, unexpected exception type.Proposed fix
+from fastapi import HTTPException + ... def test_not_found(self, mock_session): mock_session.get.return_value = None - with pytest.raises(Exception) as exc: + with pytest.raises(HTTPException) as exc: validator_config_crud.get( mock_session, TEST_VALIDATOR_ID, TEST_ORGANIZATION_ID, TEST_PROJECT_ID, ) - assert "Validator not found" in str(exc.value) + assert exc.value.status_code == 404 + assert "Validator not found" in exc.value.detail
| def create_validator( | ||
| payload: ValidatorCreate, | ||
| session: SessionDep, | ||
| organization_id: int, | ||
| project_id: int, | ||
| _: AuthDep, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the function context and parameter types
cat -n backend/app/api/routes/validator_configs.py | head -50Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 1753
🏁 Script executed:
# Search for how organization_id and project_id are handled in the codebase
rg -n "organization_id|project_id" --type=py -C3 backend/app/api/ | head -200Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 4736
🏁 Script executed:
# Check the auth validation patterns - look for how AuthDep validates tenant access
rg -n "AuthDep|SessionDep" --type=py -C2 backend/app/api/routes/ | head -150Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 4112
🏁 Script executed:
# Check if there's middleware or dependency validation for these parameters
fd -e py backend/app | xargs rg -l "organization_id.*validate|verify.*organization" --type=pyRepository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 429
🏁 Script executed:
# Look for the ValidatorCreate schema to understand parameter definitions
rg -n "class ValidatorCreate|ValidatorCreate" --type=py -C5 backend/Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 3845
🏁 Script executed:
# Find and examine the AuthDep implementation
rg -n "AuthDep" backend/app/api/deps.py -A 10Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 136
🏁 Script executed:
# Check the full deps.py file to understand dependency injection
cat -n backend/app/api/deps.pyRepository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 1438
🏁 Script executed:
# Examine CRUD methods for validation
cat -n backend/app/crud/validator_config.py | head -80Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 2872
🏁 Script executed:
# Check if session is tenant-aware
rg -n "Session|session" backend/app/crud/validator_config.py -B 2 -A 5 | head -100Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 3018
organization_id and project_id are unauthenticated query parameters with no tenant validation.
These tenant-scoping parameters are passed as plain query parameters with zero validation against the authenticated user's identity. The AuthDep dependency only validates a static bearer token—it does not extract or enforce tenant affiliation. Any authenticated caller can pass arbitrary organization_id and project_id values to create, read, update, or delete validators in any tenant. The CRUD layer accepts these parameters without access control checks, enabling cross-tenant data access.
🤖 Prompt for AI Agents
In `@backend/app/api/routes/validator_configs.py` around lines 23 - 28, The
create_validator endpoint accepts organization_id and project_id as
unauthenticated query params—fix by enforcing tenant validation: extract the
authenticated tenant identifiers from the AuthDep (e.g., user or token fields on
the AuthDep dependency used by create_validator), compare them to the incoming
organization_id and project_id, and reject the request (raise an appropriate
HTTP 403/Forbidden) if they do not match; alternatively, remove those query
params and derive organization_id/project_id from the AuthDep principal and pass
only validated IDs into the CRUD layer so ValidatorCreate handling and any calls
that use session, create_validator, and the downstream validator CRUD functions
cannot operate across tenants.
| class ValidatorUpdate(SQLModel): | ||
| model_config = ConfigDict(extra="forbid") | ||
|
|
||
| type: Optional[ValidatorType] = None | ||
| stage: Optional[Stage] = None | ||
| on_fail_action: Optional[GuardrailOnFail] = None | ||
| is_enabled: Optional[bool] = None |
There was a problem hiding this comment.
extra="forbid" on ValidatorUpdate prevents config field updates.
ValidatorCreate (via ValidatorBase) uses extra="allow", enabling callers to pass arbitrary config keys that get split into the JSONB config column. However, ValidatorUpdate uses extra="forbid", which means any config key in a PATCH payload will be rejected by Pydantic before it reaches the CRUD layer's split_validator_payload. This effectively makes config fields immutable after creation.
If config updates are intended (the CRUD update method already handles merging config fields), change this to extra="allow":
🐛 Proposed fix
class ValidatorUpdate(SQLModel):
- model_config = ConfigDict(extra="forbid")
+ model_config = ConfigDict(extra="allow")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class ValidatorUpdate(SQLModel): | |
| model_config = ConfigDict(extra="forbid") | |
| type: Optional[ValidatorType] = None | |
| stage: Optional[Stage] = None | |
| on_fail_action: Optional[GuardrailOnFail] = None | |
| is_enabled: Optional[bool] = None | |
| class ValidatorUpdate(SQLModel): | |
| model_config = ConfigDict(extra="allow") | |
| type: Optional[ValidatorType] = None | |
| stage: Optional[Stage] = None | |
| on_fail_action: Optional[GuardrailOnFail] = None | |
| is_enabled: Optional[bool] = None |
🤖 Prompt for AI Agents
In `@backend/app/schemas/validator_config.py` around lines 24 - 30,
ValidatorUpdate currently sets model_config = ConfigDict(extra="forbid"), which
blocks any arbitrary config keys in PATCH payloads and prevents them reaching
split_validator_payload/CRUD update; change ValidatorUpdate to use model_config
= ConfigDict(extra="allow") (matching ValidatorCreate/ValidatorBase) so config
fields in the incoming update are accepted and can be merged by the existing
split_validator_payload and the CRUD update method.
| class ValidatorResponse(ValidatorBase): | ||
| pass |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
ValidatorResponse lacks explicit fields for id, timestamps, etc.
ValidatorResponse inherits only type, stage, on_fail_action, is_enabled from ValidatorBase. Fields like id, organization_id, project_id, created_at, and updated_at are not declared — they only survive because extra="allow" is inherited. This makes the response schema undocumented in OpenAPI and fragile.
Consider adding the key response fields explicitly so they appear in the generated API docs:
♻️ Suggested improvement
class ValidatorResponse(ValidatorBase):
- pass
+ id: UUID
+ organization_id: int
+ project_id: int
+ created_at: datetime
+ updated_at: datetime🤖 Prompt for AI Agents
In `@backend/app/schemas/validator_config.py` around lines 33 - 34,
ValidatorResponse currently inherits only fields from ValidatorBase and relies
on extra="allow", so add explicit response fields to the ValidatorResponse model
to make them visible in OpenAPI: declare id: UUID4, organization_id: UUID4,
project_id: UUID4, created_at: datetime, and updated_at: datetime (import UUID4
from pydantic and datetime from datetime) as attributes on the ValidatorResponse
class while keeping the existing inheritance from ValidatorBase so the schema
includes both base validator properties and these persistent metadata fields.
| test_engine = create_engine( | ||
| str(settings.SQLALCHEMY_DATABASE_URI), | ||
| echo=False, | ||
| pool_pre_ping=True, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "ENVIRONMENT|POSTGRES_DB|testing|SQLALCHEMY_DATABASE_URI" --type=py -C3 backend/app/core/config.pyRepository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 1893
🏁 Script executed:
# Check if .env.test exists and show its content
find . -name ".env.test" -o -name ".env.test.*" | head -20Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 97
🏁 Script executed:
# Also check if there's validation for POSTGRES_DB being set
rg -n "POSTGRES_DB|model_validate|validate_" --type=py backend/app/core/config.py -A2 -B2Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 422
🏁 Script executed:
# Check .env.test.example content
cat -n ./.env.test.exampleRepository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 999
🏁 Script executed:
# Check Settings class definition and env_file loading behavior
rg -n "class Settings|env_file|ConfigFile" --type=py backend/app/core/config.py -A5 -B2 | head -50Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 756
Test database isolation is not validated; missing .env.test risks data loss.
The test engine uses POSTGRES_DB from environment configuration, which defaults to an empty string. While .env.test.example correctly specifies POSTGRES_DB=kaapi_guardrails_testing, the actual .env.test file does not exist in the repository. If developers fail to create .env.test from the example, POSTGRES_DB remains empty, causing the database URI to connect to PostgreSQL's default or current schema instead of an isolated test database. The clean_db fixture would then truncate tables in the wrong database.
Add validation in config.py to raise an error if POSTGRES_DB is empty when ENVIRONMENT=testing, and document that .env.test must be created before running tests.
🤖 Prompt for AI Agents
In `@backend/app/tests/conftest.py` around lines 13 - 17, When running tests the
DB name can be empty and cause destructive truncation of the wrong DB; add a
validation in config.py (where Settings or environment loading occurs) that
checks if ENVIRONMENT == "testing" and POSTGRES_DB is falsy/empty and, if so,
raise a clear RuntimeError/ValueError explaining that .env.test must be created
and POSTGRES_DB must be set; also ensure the error surfaces before
create_engine/ test setup (reference settings.SQLALCHEMY_DATABASE_URI and the
POSTGRES_DB/ENVIRONMENT settings names) and add a short comment/docs note
telling devs to copy .env.test.example to .env.test before running tests.
| def create_validator(self, client, payload_key="minimal", **kwargs): | ||
| """Helper to create a validator.""" | ||
| payload = {**VALIDATOR_PAYLOADS[payload_key], **kwargs} | ||
| return client.post(f"{BASE_URL}{DEFAULT_QUERY_PARAMS}", json=payload) | ||
|
|
||
| def get_validator(self, client, validator_id): | ||
| """Helper to get a specific validator.""" | ||
| return client.get(f"{BASE_URL}{validator_id}/{DEFAULT_QUERY_PARAMS}") | ||
|
|
||
| def list_validators(self, client, **query_params): | ||
| """Helper to list validators with optional filters.""" | ||
| params_str = f"?organization_id={TEST_ORGANIZATION_ID}&project_id={TEST_PROJECT_ID}" | ||
| if query_params: | ||
| params_str += "&" + "&".join(f"{k}={v}" for k, v in query_params.items()) | ||
| return client.get(f"{BASE_URL}{params_str}") | ||
|
|
||
| def update_validator(self, client, validator_id, payload): | ||
| """Helper to update a validator.""" | ||
| return client.patch(f"{BASE_URL}{validator_id}{DEFAULT_QUERY_PARAMS}", json=payload) | ||
|
|
||
| def delete_validator(self, client, validator_id): | ||
| """Helper to delete a validator.""" | ||
| return client.delete(f"{BASE_URL}{validator_id}/{DEFAULT_QUERY_PARAMS}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the route file and examine route definitions
find backend -name "*validator*" -type f | head -20Repository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 946
🏁 Script executed:
# Look for BASE_URL and DEFAULT_QUERY_PARAMS definitions in the test file
rg -n "BASE_URL|DEFAULT_QUERY_PARAMS" backend/app/tests/test_validator_configs_integration.pyRepository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 901
🏁 Script executed:
# Examine the route definitions for validator configs
rg -n "@app|@router" backend/app/api/routes/ -A 5 | grep -i validatorRepository: ProjectTech4DevAI/kaapi-guardrails
Length of output: 2407
Inconsistent trailing slash in URL construction across helpers.
update_validator (line 78) omits the trailing / before query params, while get_validator (line 67) and delete_validator (line 82) include it:
- Line 67:
f"{BASE_URL}{validator_id}/{DEFAULT_QUERY_PARAMS}"→/api/v1/.../configs/<id>/?org=... - Line 78:
f"{BASE_URL}{validator_id}{DEFAULT_QUERY_PARAMS}"→/api/v1/.../configs/<id>?org=... - Line 82:
f"{BASE_URL}{validator_id}/{DEFAULT_QUERY_PARAMS}"→/api/v1/.../configs/<id>/?org=...
This inconsistency should be fixed by adding the / before query params in update_validator:
Proposed fix
def update_validator(self, client, validator_id, payload):
"""Helper to update a validator."""
- return client.patch(f"{BASE_URL}{validator_id}{DEFAULT_QUERY_PARAMS}", json=payload)
+ return client.patch(f"{BASE_URL}{validator_id}/{DEFAULT_QUERY_PARAMS}", json=payload)🤖 Prompt for AI Agents
In `@backend/app/tests/test_validator_configs_integration.py` around lines 60 -
82, The update_validator helper constructs its URL without the trailing slash
before DEFAULT_QUERY_PARAMS while get_validator and delete_validator include it;
update the URL construction in update_validator to include the "/" between
validator_id and DEFAULT_QUERY_PARAMS (same pattern as
get_validator/delete_validator) so the path becomes BASE_URL + validator_id +
"/" + DEFAULT_QUERY_PARAMS, ensuring consistent URL formatting across
create_validator/get_validator/update_validator/delete_validator and using the
same BASE_URL and DEFAULT_QUERY_PARAMS symbols.
| overlap = set(model_fields) & set(config_fields) | ||
| if overlap: | ||
| raise ValueError(f"Config keys conflict with reserved field names: {overlap}") |
There was a problem hiding this comment.
Overlap check is unreachable — the if/else on lines 21–24 guarantees mutual exclusivity.
Every key is routed to exactly one of model_fields or config_fields, so set(model_fields) & set(config_fields) is always empty and this ValueError can never be raised.
If the intent is to validate something else (e.g., config keys that shadow system field names from an external source), this needs a different approach. Otherwise, consider removing the dead check to avoid confusion.
🤖 Prompt for AI Agents
In `@backend/app/utils.py` around lines 26 - 28, The overlap check computing
overlap = set(model_fields) & set(config_fields) and raising ValueError is dead
code because earlier routing guarantees mutual exclusivity; remove the overlap
calculation and the conditional that raises the ValueError (or replace it with
the intended validation if you meant to check an external source), and ensure
any intended shadowing validation targets the correct input (e.g., compare
external_config_keys against model_fields instead of config_fields) so only
meaningful checks remain; search for model_fields, config_fields, and overlap to
locate and update the code.
Pull request was closed
1 participant
Summary
Target issue is #28.
Explain the motivation for making this change. What existing problem does the pull request solve?
We don't want to populate the validator_log each time a validator has parsed a message. There will be a flag
include_all_validator_logswhich if disabled will populate only failed validator logs. Otherwise, we will populate all logs to the validator_log table.Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit
New Features
Database
Tests